feat(rpc): finalize SyncChainMmr gRPC endpoint#2075
Conversation
22c2bad to
e76a4b9
Compare
3e162ee to
7b6e616
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
89d9bfa to
04bfe3e
Compare
igamigo
left a comment
There was a problem hiding this comment.
Sorry for the late review. Implementation looks great but noticed very small doc inconsistencies while reading through the changes
| // Data needed to update the partial MMR from `request.block_range.block_from + 1` to | ||
| // `response.block_range.block_to` or the chain tip. | ||
|
|
||
| // Data needed to update the partial MMR from `request.block_range.current_block_height + 1` to |
There was a problem hiding this comment.
nit: Probably should just be request.current_block_height + 1
| CHAIN_TIP_UNSPECIFIED = 0; | ||
| // Finality level we'd like to sync up to. | ||
| enum FinalityLevel { | ||
| FINALITY_LEVEL_UNSPECIFIED = 0; |
There was a problem hiding this comment.
Should we clarify here or elsewhere that choosing "unspecified" falls back to committed? There's a small ambiguity in the docs because the request proto comment says "either the committed or the proven tip" which does not cover the unspecified flow either
There was a problem hiding this comment.
iirc UNSPECIFIED usually means you did not specify and therefore the server is free to do whatever it wants.
| // Block number from which to synchronize (inclusive). Set this to the last block | ||
| // already present in the caller's MMR so the delta begins at the next block. | ||
| fixed32 block_from = 1; | ||
|
|
||
| // Upper bound for the block range. Determines how far ahead to sync. | ||
| oneof upper_bound { | ||
| // Sync up to this specific block number (inclusive), clamped to the committed chain tip. | ||
| fixed32 block_num = 2; | ||
| // Sync up to a chain tip variant (committed or proven). | ||
| ChainTip chain_tip = 3; | ||
| } | ||
| fixed32 current_block_height = 1; |
There was a problem hiding this comment.
nit: I think a potentially better name is current_sync_height or maybe current_client_block_height. Also not from this PR but not sure "(inclusive)" here is correct? It's not really part of the response, right?
There was a problem hiding this comment.
I think removing "(inclusive)" would make sense.
Regarding the rename -- I think current_sync_height is not really precise (I'd definitely include block in the name).
I'm fine either with the current name or current_client_block_height as well -- I like the latter because it makes it explicit that it's the client state that is communicated in that field.
| data_directory: PathBuf, | ||
| iterations: usize, | ||
| concurrency: usize, | ||
| block_range_size: u32, |
There was a problem hiding this comment.
Seems like this was removed but doc comments were not updated to reflect this.
Based on the discussion in PR #2069 and issue #2044 we've converged on some smaller changes to the
SyncChainMmrendpoint.Changes made to the current interface:
The proposed protobuf interface (implemented by this PR) is the following:
Closes #2044